Add precommit and run precommit on all files by tomelm · Pull Request #26 · graphql-python/flask-graphql

tomelm · 2017-03-17T23:09:27Z

Adds pre-commit and runs it on all the current files. I skipped a few validations so I'm not making too many big changes and also set the max-line-length to 120:

(venv) flask-graphql|add-precommit ⇒ git commit -am "Add precommit and run precommit on all files"
autopep8 wrapper.........................................................Passed
Check for added large files..............................................Passed
Check python ast.........................................................Passed
Check for byte-order marker..............................................Passed
Check for case conflicts.................................................Passed
Check docstring is first.................................................Passed
Check JSON...........................................(no files to check)Skipped
Check for merge conflicts................................................Passed
Check Yaml...............................................................Passed
Debug Statements (Python)................................................Passed
Fix double quoted strings................................................Passed
Fix End of Files.........................................................Passed
Flake8...................................................................Failed
hookid: flake8

tests/test_graphqlview.py:38:1: E731 do not assign a lambda expression, use a def
tests/test_graphqlview.py:39:1: E731 do not assign a lambda expression, use a def
tests/test_graphqlview.py:452:1: F811 redefinition of unused 'test_supports_pretty_printing' from line 328

Fix python encoding pragma...............................................Passed
Tests should end in _test.py.............................................Failed
hookid: name-tests-test

tests/schema.py does not match pattern ".*_test.py"
tests/test_graphqlview.py does not match pattern ".*_test.py"
tests/test_graphiqlview.py does not match pattern ".*_test.py"
tests/app.py does not match pattern ".*_test.py"

Pretty format JSON...................................(no files to check)Skipped
Fix requirements.txt.....................................................Passed
Trim Trailing Whitespace.................................................Passed
pyupgrade................................................................Passed
Reorder python imports...................................................Passed

coveralls · 2017-03-17T23:10:36Z

Coverage increased (+0.1%) to 96.629% when pulling 93ca54f on tomelm:add-precommit into 724695a on graphql-python:master.

coveralls · 2017-03-17T23:10:36Z

Coverage increased (+0.1%) to 96.629% when pulling 93ca54f on tomelm:add-precommit into 724695a on graphql-python:master.

coveralls · 2017-03-17T23:10:36Z

Coverage increased (+0.1%) to 96.629% when pulling 93ca54f on tomelm:add-precommit into 724695a on graphql-python:master.

chriskuehl

lgtm. often we add pre-commit run --all-files to the tests somewhere which helps to make sure it doesn't regress, but it's up to you if you want to do that or not.

chriskuehl · 2017-03-17T23:19:17Z

requirements.txt

@@ -0,0 +1 @@
+pre-commit==0.13.3


I'd probably call this file requirements-dev.txt since you don't actually need pre-commit to be installed in production (and that's what requirements.txt implies, even if you're not actually doing it in this case).

Also, we usually don't pin specific versions for our dev dependencies, but that's totally up to you / the project's stance on that.

Maybe adding something like the following in the README will actually add more visibility into Yelp's pre-commit project and provide a better understanding to the developer, rather than using a requirements.txt file that might confuse collaborators :)

Collaborate

This project is using pre-commit to help the developer run pre-commit hooks easily, allowing to autoformat Python files, reorder the imports, and other various checks.

You can easily start using it via pip install pre-commit.

Thoughts?

syrusakbary · 2017-03-18T16:41:38Z

.pre-commit-config.yaml

+-   repo: https://github.com/asottile/reorder_python_imports
+    sha: v0.3.2
+    hooks:
+    -   id: reorder-python-imports


I've been using isort for checking the import ordering: https://github.com/graphql-python/flask-graphql/blob/master/tox.ini#L30.
Also being used in the automatic linters in graphql-core https://github.com/graphql-python/graphql-core/blob/master/bin/autolinter#L7, and graphene https://github.com/graphql-python/graphene/blob/master/bin/autolinter#L7.

I kind of prefer it over render-python-imports as it seems it does a smarter import grouping, not requiring a import statement per imported var.

If we could use isort instead of render-python-imports would be great! :)

syrusakbary · 2017-03-18T16:52:21Z

Other than the comments, everything is looking great... thanks for the work @tomelm! :)

Add precommit and run precommit on all files

93ca54f

chriskuehl reviewed Mar 17, 2017

View reviewed changes

syrusakbary reviewed Mar 18, 2017

View reviewed changes

syrusakbary force-pushed the master branch from 0ed9ee6 to 068ee71 Compare June 5, 2018 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add precommit and run precommit on all files#26

Add precommit and run precommit on all files#26
tomelm wants to merge 1 commit intographql-python:masterfrom
tomelm:add-precommit

tomelm commented Mar 17, 2017

Uh oh!

coveralls commented Mar 17, 2017

Uh oh!

coveralls commented Mar 17, 2017

Uh oh!

coveralls commented Mar 17, 2017

Uh oh!

chriskuehl left a comment

Uh oh!

chriskuehl Mar 17, 2017

Uh oh!

chriskuehl Mar 17, 2017

Uh oh!

syrusakbary Mar 18, 2017 •

edited

Loading

Uh oh!

syrusakbary Mar 18, 2017 •

edited

Loading

Uh oh!

syrusakbary commented Mar 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@@ -0,0 +1 @@
		pre-commit==0.13.3

Conversation

tomelm commented Mar 17, 2017

Uh oh!

coveralls commented Mar 17, 2017

Uh oh!

coveralls commented Mar 17, 2017

Uh oh!

coveralls commented Mar 17, 2017

Uh oh!

chriskuehl left a comment

Choose a reason for hiding this comment

Uh oh!

chriskuehl Mar 17, 2017

Choose a reason for hiding this comment

Uh oh!

chriskuehl Mar 17, 2017

Choose a reason for hiding this comment

Uh oh!

syrusakbary Mar 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Collaborate

Uh oh!

syrusakbary Mar 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syrusakbary commented Mar 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

syrusakbary Mar 18, 2017 •

edited

Loading

syrusakbary Mar 18, 2017 •

edited

Loading